Option to have a Tsd as an index #314
Conversation
|
I think it's good and I would keep it only for time series. |
Co-authored-by: Guillaume Viejo <[email protected]>
|
Everything should have been addressed now, let me know if there's anything else I should fix |
|
That's great. I would still add one test for it. |
|
Excellent point, adding tests I actually realised I did not set the behavior for This should be now be good to go! |
|
It's missing for TsdTensor I think? |
|
Oh sorry maybe I misunderstood one of your early messages - do you thing this should be implemented for Tsdtensors as well? |
|
Or you mean I should exclude tensors? |
|
Yes I would implement it for TsdTensor too |
|
I'll come back to this soon if still unaddressed! Since I am at it, I'll ask a slightly related but separate question: why is |
|
Basically TsIndex is like pd.Index in pandas. |
|
This should now good to go! I did a minor refactoring of test classes for readability, hopefully that's fine. Also, I'd love to do a separate PR for tests cleanup if that is good for you! |
Co-authored-by: Guillaume Viejo <[email protected]>
Co-authored-by: Guillaume Viejo <[email protected]>
Co-authored-by: Edoardo Balzani <[email protected]>
…apple into tsdataframe-index # Conflicts: # pynapple/core/time_series.py
|
Let mw know if anything else is needed here! (In the test refactoring, move test generated files to temporary folders would be very useful to avoid spurious commits) |
|
Thanks I will look into it as soon as possible. |
There was a problem hiding this comment.
It's almost good. Is it possible to add this for all time series :
if not all(
np.issubdtype(k.dtype, np.bool_) if isinstance(k, Tsd) else True
for k in key
):
raise ValueError(
"When indexing with a Tsd, it must contain boolean values"
)
I should only be boolean indexing with another Tsd otherwise it doesn't make sense to index with integers because it means you are breaking the relationship between the time index and the corresponding values. Somehow it's similar to shuffling but it should be a proper method in the shuffling module.
|
Ok, done it! I also change a bit the logic to bit more interpretable, and fixed the tests accordingly |
# Conflicts: # pynapple/core/time_series.py # tests/test_time_series.py
|
Should have fixed stuff and rebased on dev head. Would be great if we can merge so that I won't have to resolve more conflicts in the future! |
|
Looks good thank you Luigi |
When working with Tsds, I might sometimes write, eg for filtering:
I think it would be nice and not confusing for how the class works atm (very thorough feature equivalence with np.ndarray) having instead:
Just by changing the setter and getter for the
Tsd.Would something like this make sense? This is just a draft for
Tsd, it could be extended to other classes maybe although I never encountered the need.